fix: HTTPS Redirects Always Enabled #22
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughHelm chart version bumped from 0.2.18 to 0.2.19. In the Mailpit HTTPRoute template, the conditional rendering now uses an explicit equality check against the string "true" for the included "lfx-platform.https-enabled" value. Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm
participant Tmpl as Template: https-redirect-httproute.yaml
participant Inc as include "lfx-platform.https-enabled"
participant Out as Rendered HTTPRoute
Helm->>Tmpl: Render chart
Tmpl->>Inc: Evaluate https-enabled
Inc-->>Tmpl: "true" | "false" | other
Tmpl->>Tmpl: Check eq(result, "true") and other conditions
alt eq(...) is true
Tmpl-->>Out: Emit HTTPRoute manifest
else eq(...) is false
Tmpl-->>Out: Skip emitting HTTPRoute
end
note over Tmpl: Conditional now uses explicit string equality check
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where HTTPS redirect HTTPRoutes for Authelia and Mailpit were being created even when HTTPS was disabled. The issue occurred because Helm's and function evaluates any non-empty string as true, and the lfx-platform.https-enabled template was returning the string "false" instead of a boolean false.
- Fixed conditional logic to properly check if HTTPS is enabled using string comparison
- Added missing traefik.enabled condition to the authelia template for consistency
- Ensures HTTPS redirects are only created when HTTPS is actually configured
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| charts/lfx-platform/templates/mailpit/https-redirect-httproute.yaml | Updated conditional to use string comparison for HTTPS check |
| charts/lfx-platform/templates/authelia/https-redirect-httproute.yaml | Fixed HTTPS check and added missing traefik.enabled condition |
charts/lfx-platform/templates/authelia/https-redirect-httproute.yaml
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
charts/lfx-platform/templates/mailpit/https-redirect-httproute.yaml (1)
4-4: Consider parsing the helper output as a boolean instead of string-matching
Relying on a literal string"true"couples every consumer to the helper’s exact output. A more robust pattern is:{{ if and .Values.mailpit.enabled .Values.traefik.enabled (include "lfx-platform.https-enabled" . | fromYaml | default false) -}}
fromYamlcoerces"true"/"false"to booleans, shielding you if the helper ever returns real YAML booleans or gets refactored.charts/lfx-platform/templates/authelia/https-redirect-httproute.yaml (1)
4-4: Avoid duplicating complex conditions across components
Mailpit and Authelia now share almost identical guard clauses. Extracting a helper, e.g.{{- define "lfx-platform.enable-https-redirect" -}} … {{- end -}}, would centralise the logic and cut drift risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/lfx-platform/templates/authelia/https-redirect-httproute.yaml(1 hunks)charts/lfx-platform/templates/mailpit/https-redirect-httproute.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (2)
charts/lfx-platform/templates/mailpit/https-redirect-httproute.yaml (1)
4-4: Explicit equality check fixes the unintended redirect creation
Changing the condition toeq ... "true"prevents the previous truthy string pit-fall, so the HTTPRoute now renders only when an HTTPS listener really exists.charts/lfx-platform/templates/authelia/https-redirect-httproute.yaml (1)
4-4: Good addition of the Traefik gate and strict HTTPS check
The extra.Values.traefik.enabledguard plus the explicit"true"comparison close the loophole that rendered a redirect with no HTTPS listener.
This change ensures the https redirect for mailipit is only created when there is a https listener defined on the gateway. Generated with [Cursor](https://cursor.com/) Signed-off-by: Trevor Bramwell <[email protected]>
bramwelt
force-pushed
the
bramwelt/fix-https-redirects
branch
from
cb1b412 to
0a025c4
Compare
Signed-off-by: Trevor Bramwell <[email protected]>
This change ensures the https redirects for mailipit
aren't created when there is no https listener defined on the gateway.
andevaluates any non-empty string as true, and the include wasreturning the string "false".
Generated with Cursor
Signed-off-by: Trevor Bramwell [email protected]